engine: do not stall the host command queue on a failed send in Agent… - #14040

@blueorangutan package

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 17.21%. Comparing base (2cd8c5e) to head (4d8e427).

@@             Coverage Diff              @@
##               4.20   #14040      +/-   ##
============================================
+ Coverage     16.34%   17.21%   +0.86%     
- Complexity    13574    13580       +6     
============================================
  Files          5669     5264     -405     
  Lines        501368   468343   -33025     
  Branches      60903    55005    -5898     
============================================
- Hits          81964    80632    -1332     
+ Misses       410219   378694   -31525     
+ Partials       9185     9017     -168     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 19095

@nagaboinaramgopal , an compile error in your test code:

09:17:39 [ERROR] /jenkins/workspace/acs-centos8-pkg-builder/dist/rpmbuild/BUILD/cloudstack-4.20.4.0-SNAPSHOT/engine/orchestration/src/test/java/com/cloud/agent/manager/AgentAttacheSendNextTest.java:[38,12] error: constructor AgentAttache in class AgentAttache cannot be applied to given types;
09:17:39   required: AgentManagerImpl,long,String,String,boolean
09:17:39   found: <null>,long,String,String,<null>,boolean
09:17:39   reason: actual and formal argument lists differ in length
09:17:39 [INFO] 1 error

can you have a look?

@nagaboinaramgopal , an compile error in your test code:

09:17:39 [ERROR] /jenkins/workspace/acs-centos8-pkg-builder/dist/rpmbuild/BUILD/cloudstack-4.20.4.0-SNAPSHOT/engine/orchestration/src/test/java/com/cloud/agent/manager/AgentAttacheSendNextTest.java:[38,12] error: constructor AgentAttache in class AgentAttache cannot be applied to given types;
09:17:39   required: AgentManagerImpl,long,String,String,boolean
09:17:39   found: <null>,long,String,String,<null>,boolean
09:17:39   reason: actual and formal argument lists differ in length
09:17:39 [INFO] 1 error

can you have a look?

fixed and pushed, Thanks Daan

Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 19123

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✖️ debian ✔️ suse15. SL-JID 19128

@nagaboinaramgopal , as you are not referring an issue, can you add a reporduction scheme/scenario?

@nagaboinaramgopal , as you are not referring an issue, can you add a reporduction scheme/scenario?

No linked issue on this one, it came from following the in-sequence command path in AgentAttache, so here is the scenario, and it reproduces deterministically as a fail-before / pass-after test.

For a given host, in-sequence commands run one at a time, and _currentSequence holds the sequence that is currently in flight. While it is non-null, any newly arriving in-sequence command is queued in _requests rather than sent (the executeInSequence() && _currentSequence != null branch in send(...)).

Sequence that stalls the queue:

1. Command A is in flight to the host, so _currentSequence = A. Commands B and C arrive meanwhile and queue behind it.
2. A completes, so the answer path calls sendNext(...), which pops B and calls send(B).
3. send(B) throws AgentUnavailableException. This is the transient case the catch block exists for: the agent link is briefly down or reconnecting, or a socket write fails as the command goes out.
4. Before this change, sendNext cancelled B but then still ran _currentSequence = B at the end of the method. B was just cancelled, so no answer will ever arrive for it.
5. _currentSequence now points at a dead sequence. C, and every later in-sequence command to this host, takes the _currentSequence != null path and waits for B. The queue only unwedges when the outer send(req, wait) hits its full timeout and clears _currentSequence, so in the meantime each in-sequence command to that host eats a timeout.

AgentAttacheSendNextTest.sendNextAdvancesPastAFailedCommandToTheNextQueued pins this: it queues a command whose send() fails followed by a good one, then asserts sendNext dispatches the good command and leaves _currentSequence on it.

On the unpatched code the test fails, which is exactly the stall:

java.lang.AssertionError: the next queued command should have been dispatched expected:<200> but was:<null>
Tests run: 1, Failures: 1

With the change it passes (the queued command is dispatched instead of the queue parking on the cancelled one):

Tests run: 1, Failures: 0

I could not tie it to a specific cluster incident, since it needs a send failure at the exact moment the queue is drained, but that is the path and the test reproduces it deterministically.